-
-
Notifications
You must be signed in to change notification settings - Fork 264
feat(tracing): migrate logging to tracing #1308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
==========================================
+ Coverage 43.30% 43.59% +0.30%
==========================================
Files 23 23
Lines 2245 2230 -15
==========================================
Hits 972 972
+ Misses 1273 1258 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
10 seconds ago I was happy that my GitHub notifications were clear and then Shingo comes in with a crazy PR!!!!! +0 additions -0 deletions though. Love it. LGTM |
|
planning to do any progress on this soon? (added to 2.12.0 milestone) |
Yeah, I'm planning to open a PR later this week. |
|
yup, 2.12.0 will happen in 2026 :) |
|
Sorry for the delay. I'll start working on this PR today. |
|
No worries at all! |
5bb0f7b to
f6b930d
Compare
f6b930d to
b273940
Compare
b273940 to
b1e9f80
Compare
|
This is ready for review now @orhun . I consider this a first draft, and there are likely areas open for discussion. When you have time, I'd really appreciate your thoughts and feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO:
- Replace all ASCII color codes with
owo_colors. - Consider how to handle Windows and non-TTY environments (this may also have been an issue in the previous implementation).
75b4d8d to
b53b100
Compare
| static MAX_MODULE_WIDTH: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| /// Unicode braille spinner frames used by indicatif. | ||
| const SPINNER_TICKS: &[&str] = &["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we could keep the old arrow-styled progress indicator (for the first line in the logs at least)
I really like the detailed logging btw!
Let me know when this is ready for review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we could keep the old arrow-styled progress indicator (for the first line in the logs at least)
That should be doable.
We can bring back the old arrow-style progress indicator, but when implemented on top of tracing / span-based instrumentation via tracing-indicatif, it would look slightly different from the previous approach:
If that's acceptable, I'm happy to revert the change accordingly. In addition to the progress indicator itself, we can of course also emit explicit logs at the beginning and end of the processing step.
In an ideal world, I'd like to include a progress indicator with percentages as well. However, with the current design, where spans and logging live inside the core SDK, that starts to get a bit messy from a design perspective. It's technically possible, but I intentionally avoided it for now to keep the core clean.
Let me know when this is ready for review
Regarding ANSI/ASCII color codes on Windows or non-TTY environments: since we were already using colored output before, I think it's reasonable to review this as-is for now.
If you're open to it, I'd really appreciate review feedback on the following points in the context of adding this to git-cliff-core as an SDK:
- Which functions should be annotated with
#[instrument](all vs. a subset) - Whether
spannames should be explicitly set - Which fields are appropriate to capture on spans
From a "keep git-cliff-core as a clean SDK" perspective, another option could be to explore whether progress reporting can be handled in a less hacky way on top of env_logger, if that's feasible. Just putting it out there as an alternative.
b53b100 to
7367d4b
Compare
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
…ntics Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
Signed-off-by: Shingo OKAWA <[email protected]>
7367d4b to
9ff9ae5
Compare
Description
This PR migrates the logging implementation from
env_loggertotracing.The migration preserves the existing log format while extending it to include span information, enabling more structured and contextual logging without changing the default user experience.
Motivation and Context
This change is motivated by the following discussion and issues:
env_loggertotracing+tracing-subscriber#1268The goal is to modernize
git-cliff's logging infrastructure by adoptingtracing, while maintaining backward compatibility and avoiding unnecessary complexity in the core crate.What Has Changed
git-cliff-corenow depends ontracingbehind a feature flag.logoutput is produced.tracing-based implementation usingtracing-indicatif.tracing, implementing such behavior cleanly would require API changes outside ofgit-cliff-core.How Has This Been Tested?
Screenshots / Logs (if applicable)
N/A
Types of Changes
Checklist: